RFC: Add SetRuntimeEnvironment call for NI-DCPower #962

ni-jfitzger · 2023-07-17T22:31:57Z

What does this Pull Request accomplish?

Add an NiDCPowerLibrary::SetRuntimeEnvironment call upon library load, before init is called.

Implemented via the following changes:

Update source/codegen/metadata/nidcpower/functions_addon.py to privately add the SetRuntimeEnvironment function
Update source/codegen/metadata/nidcpower/__init__.py so that the new metadata will be merged. I merged metadata for attributes, functions and enums. I didn't mess with config_addon.py, though.
Update library.cpp.mako to call SetRuntimeEnvironment if the function exists
Generate a version.h with some of the values found in version.rc, defined as string constants (couldn't figure out any other way to get access to the data easily)
Update CMakeLists.txt and generate

Why should this Pull Request be merged?

As developers, we want to know how much nigrpc_device_server is used and which versions.

What testing has been done?

Built ni_grpc_device_server.exe locally and called it with the nimi-python system tests in CEIP test mode.
- Verified that the Runtime Environment data was logged correctly.
Manually launched the server and called it, opening and closing sessions, as well as the python process that I called it from, multiple times. Confirmed that data was only logged once. This makes sense because the service that loads the library is supposed to remain loaded for the lifetime of the server instance.

TODOs:

Before submitting this, merge changes from main after Add SetRuntimeEnvironment to all MI Driver and NI-Fake library classes #964 is submitted and add a unit test. I might just close this and create a separate PR.

ni-jfitzger · 2023-07-17T22:32:59Z

TODO: update tests? Maybe testing should be done with nifake?

ni-jfitzger · 2023-07-17T22:37:11Z

source/codegen/templates/library.cpp.mako

@@ -44,6 +45,10 @@ ${service_class_prefix}Library::${service_class_prefix}Library() : shared_librar
 %>\
  function_pointers_.${method_name} = reinterpret_cast<${method_name}Ptr>(shared_library_.get_function_pointer("${c_name}"));
 % endfor
+% if 'SetRuntimeEnvironment' in service_helpers.filter_api_functions(functions, only_mockable_functions=False):
+
+  this->SetRuntimeEnvironment(nidevice_grpc::NIDEVICE_GRPC_ORIGINALFILENAME.c_str(), nidevice_grpc::NIDEVICE_GRPC_FILEVERSION.c_str(), "", "");


Maybe I need to do a try, catch in case this errors?

Rather than adding specific logic for a specific function, it might be better to define this requirement in the metadata and use that to add any extra "initialization functions" so that this is extensible to other functions in the future. I'll defer to @reckenro or @astarche on that since they know more about the code generation, though.

This isn't Python where you ask for forgiveness, not for permission :)
Best not to throw unless an error truly happened.

I think ideally, you have a well documented function that only calls into the driver if the function is indeed present. That means you don't rely on the codegenerated wrapper as-is.

I don't know how accessible the things you would need are. If they aren't then maybe it's fine to try/catch, not the end of the world.

Like Marcos, I was expecting something like:

if (function_pointers_.SetRuntimeEnvironment) { function_pointers_.SetRuntimeEnvironment(environment, environmentVersion, reserved1, reserved2); }

But I agree that try/catch is not the end of the world.

I'm always a fan of adding metadata conventions. But I think there's already a strong convention in the drivers to implement/use SetRuntimeEnvironment in this way and that it's implemented similarly in nimi-python so I'm happy for the team to use their judgement on it.

Is it possible that we'd want to automatically add SetRuntimeEnvironment even if drivers don't declare it in their metadata? If the convention is strong enough, it can be better to have it be global.

I actually just added that check.
Let me know if you think I need to document the reason for the call.

Rather than adding specific logic for a specific function, it might be better to define this requirement in the metadata and use that to add any extra "initialization functions" so that this is extensible to other functions in the future. I'll defer to @reckenro or @astarche on that since they know more about the code generation, though.

I'll stick with the YAGNI (You Ain't Gonna Need It) principle and avoid overcomplicating it, since I haven't been told other wise.

source/codegen/metadata/nidcpower/__init__.py

ni-jfitzger · 2023-07-17T22:41:53Z

I'm not sure why the validate_python check failed. I don't think it's related to my changes.

SparkingSpork · 2023-07-17T22:49:13Z

source/codegen/templates/library.cpp.mako

@@ -44,6 +45,10 @@ ${service_class_prefix}Library::${service_class_prefix}Library() : shared_librar
 %>\
  function_pointers_.${method_name} = reinterpret_cast<${method_name}Ptr>(shared_library_.get_function_pointer("${c_name}"));
 % endfor
+% if 'SetRuntimeEnvironment' in service_helpers.filter_api_functions(functions, only_mockable_functions=False):
+
+  this->SetRuntimeEnvironment(nidevice_grpc::NIDEVICE_GRPC_ORIGINALFILENAME.c_str(), nidevice_grpc::NIDEVICE_GRPC_FILEVERSION.c_str(), "", "");


Rather than adding specific logic for a specific function, it might be better to define this requirement in the metadata and use that to add any extra "initialization functions" so that this is extensible to other functions in the future. I'll defer to @reckenro or @astarche on that since they know more about the code generation, though.

CMakeLists.txt

bkeryan · 2023-07-17T23:07:58Z

I'm not sure why the validate_python check failed. I don't think it's related to my changes.

black 23.7 was released a week ago and it dropped Python 3.7 support.

validate_examples.py generates temporary Poetry projects on the fly, which is kind of unusual. It sounds like poetry add (at least in Poetry 1.2.2) decides to depend on black = "^23.7.0" before it realizes this won't work with Python 3.7. The easiest solution is probably to upgrade the Python version for the GitHub workflow to 3.8 or later.

Cc: @mshafer-NI

generated/version.h

generated/nidcpower/nidcpower_library.h

source/server/version.h.in

source/codegen/metadata/nidcpower/functions_addon.py

generated/nidcpower/nidcpower_library.cpp

marcoskirsch · 2023-07-18T15:53:48Z

source/codegen/templates/library.cpp.mako

@@ -44,6 +45,10 @@ ${service_class_prefix}Library::${service_class_prefix}Library() : shared_librar
 %>\
  function_pointers_.${method_name} = reinterpret_cast<${method_name}Ptr>(shared_library_.get_function_pointer("${c_name}"));
 % endfor
+% if 'SetRuntimeEnvironment' in service_helpers.filter_api_functions(functions, only_mockable_functions=False):
+
+  this->SetRuntimeEnvironment(nidevice_grpc::NIDEVICE_GRPC_ORIGINALFILENAME.c_str(), nidevice_grpc::NIDEVICE_GRPC_FILEVERSION.c_str(), "", "");


This isn't Python where you ask for forgiveness, not for permission :)
Best not to throw unless an error truly happened.

I think ideally, you have a well documented function that only calls into the driver if the function is indeed present. That means you don't rely on the codegenerated wrapper as-is.

I don't know how accessible the things you would need are. If they aren't then maybe it's fine to try/catch, not the end of the world.

ni-jfitzger · 2023-07-18T15:55:20Z

I'm not sure why the validate_python check failed. I don't think it's related to my changes.

black 23.7 was released a week ago and it dropped Python 3.7 support.

validate_examples.py generates temporary Poetry projects on the fly, which is kind of unusual. It sounds like poetry add (at least in Poetry 1.2.2) decides to depend on black = "^23.7.0" before it realizes this won't work with Python 3.7. The easiest solution is probably to upgrade the Python version for the GitHub workflow to 3.8 or later.

Cc: @mshafer-NI

Created #963 to address the issue.

* Use constexpr to avoid codebloat * Follow naming conventions * Eliminate unused constants * Add comment to CMakeLists.txt

source/codegen/templates/library.cpp.mako

ni-jfitzger · 2023-07-20T13:52:34Z

I've been thinking about how to test that SetRuntimeEnvironment is called when it should be and not called when it shouldn't be.

A mock of the NiFakeLibrary constructor (where the SetRuntimeEnvironment call happens) won't do anything except what the caller tells it to do.
If I move the SetRuntimeEnvironment call up a level into the function that calls the constructor, well that function creates the library object, itself, so I can't pass in a mock of the Library class.
The SharedLibrary object that loads the dll is constructed by the NiFakeLibrary constructor, so I can't pass in a fake or mock of SharedLibrary to noop the library loading.

@reckenro @astarche How would you feel about me tweaking the Library classes to have the SharedLibrary type passed to the constructor (so that a test could pass a mock or fake)?

astarche · 2023-07-20T19:29:20Z

@ni-jfitzger @reckenro Passing in the shared library sounds alright to me. I believe you could make it a unique_ptr and pass it in via make_unique. A shared_library abstraction is a reasonable place to introduce a pointer for testability.

Seems more likely that this code would fail because shared_library doesn't do what you expect rather than because your if statement doesn't do what you expect. But we haven't invested in building some dummy dll for testing shared_library so now is not the time to do it. We typically fall back to relying on driver-level acceptance tests, but in this case I guess we don't have a good way to test that either (both hard to verify the call and hard to find a driver that doesn't support it).

So overall: sounds good.

ni-jfitzger · 2023-07-20T19:43:58Z

This PR is being abandonded in favor of #973

bkeryan · 2023-07-20T20:22:33Z

Passing in the shared library sounds alright to me. I believe you could make it a unique_ptr and pass it in via make_unique.

Note that the NifooLibrary constructor will move the test's MockSharedLibrary, invalidating it. shared_ptr may be simpler, especially if you need to access the mock after the NifooLibrary takes ownership.

But we haven't invested in building some dummy dll for testing shared_library so now is not the time to do it.

With mocking, you don't need a dummy DLL. You can set up the MockSharedLibrary to return pointers to functions implemented in the test EXE. However, it still seems like a relatively low value test.

We typically fall back to relying on driver-level acceptance tests, but in this case I guess we don't have a good way to test that either (both hard to verify the call and hard to find a driver that doesn't support it).

Is it hard to write a C++ test that logs ETW events? In C#, we have test utilities for writing tests that capture ETW events for CEIP or whatever.

ni-jfitzger added 3 commits July 15, 2023 11:10

Add setRuntimeEnvironment as a private NI-DCPower function

fbe8af2

Call SetRuntimeEnvironment

3d582a3

Use FILEVERSION, instead

3bdebe4

ni-jfitzger requested review from reckenro, astarche and SparkingSpork as code owners July 17, 2023 22:31

ni-jfitzger requested a review from marcoskirsch July 17, 2023 22:32

ni-jfitzger commented Jul 17, 2023

View reviewed changes

source/codegen/metadata/nidcpower/__init__.py Show resolved Hide resolved

SparkingSpork approved these changes Jul 17, 2023

View reviewed changes

bkeryan reviewed Jul 17, 2023

View reviewed changes

generated/version.h Outdated Show resolved Hide resolved

marcoskirsch requested changes Jul 18, 2023

View reviewed changes

ni-jfitzger added 2 commits July 18, 2023 12:20

Address Feedback:

6187736

* Use constexpr to avoid codebloat * Follow naming conventions * Eliminate unused constants * Add comment to CMakeLists.txt

Conditionally call SetRuntimeEnvironment

2e7c120

bkeryan approved these changes Jul 18, 2023

View reviewed changes

reckenro reviewed Jul 18, 2023

View reviewed changes

source/codegen/templates/library.cpp.mako Show resolved Hide resolved

astarche approved these changes Jul 18, 2023

View reviewed changes

ni-jfitzger mentioned this pull request Jul 18, 2023

Add SetRuntimeEnvironment to all MI Driver and NI-Fake library classes #964

Merged

ni-jfitzger added 2 commits July 18, 2023 16:59

Make constants static, just in case

e56b3a2

Conditionally include version.h

6eb1120

reckenro approved these changes Jul 18, 2023

View reviewed changes

ni-jfitzger commented Jul 19, 2023

View reviewed changes

source/codegen/templates/library.cpp.mako Outdated Show resolved Hide resolved

Fix argument order

310ac3f

ni-jfitzger mentioned this pull request Jul 20, 2023

Add SetRuntimeEnvironment call to library constructors #973

Merged

ni-jfitzger closed this Jul 20, 2023

ni-jfitzger deleted the nidcpower-add-setRuntimeEnvironment-call branch August 7, 2023 14:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Add SetRuntimeEnvironment call for NI-DCPower #962

RFC: Add SetRuntimeEnvironment call for NI-DCPower #962

ni-jfitzger commented Jul 17, 2023 •

edited

Loading

ni-jfitzger commented Jul 17, 2023

ni-jfitzger Jul 17, 2023

SparkingSpork Jul 17, 2023

marcoskirsch Jul 18, 2023

astarche Jul 18, 2023

ni-jfitzger Jul 18, 2023

ni-jfitzger Jul 18, 2023

ni-jfitzger commented Jul 17, 2023

SparkingSpork Jul 17, 2023

bkeryan commented Jul 17, 2023

marcoskirsch Jul 18, 2023

ni-jfitzger commented Jul 18, 2023

ni-jfitzger commented Jul 20, 2023 •

edited

Loading

astarche commented Jul 20, 2023

ni-jfitzger commented Jul 20, 2023

bkeryan commented Jul 20, 2023

RFC: Add SetRuntimeEnvironment call for NI-DCPower #962

RFC: Add SetRuntimeEnvironment call for NI-DCPower #962

Conversation

ni-jfitzger commented Jul 17, 2023 • edited Loading

What does this Pull Request accomplish?

Why should this Pull Request be merged?

What testing has been done?

ni-jfitzger commented Jul 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ni-jfitzger commented Jul 17, 2023

Choose a reason for hiding this comment

bkeryan commented Jul 17, 2023

Choose a reason for hiding this comment

ni-jfitzger commented Jul 18, 2023

ni-jfitzger commented Jul 20, 2023 • edited Loading

astarche commented Jul 20, 2023

ni-jfitzger commented Jul 20, 2023

bkeryan commented Jul 20, 2023

ni-jfitzger commented Jul 17, 2023 •

edited

Loading

ni-jfitzger commented Jul 20, 2023 •

edited

Loading